Skip to content

Return a typed PipelineResult with an execution trace (#332) - #442

Merged
jeremymanning merged 3 commits into
mainfrom
feat/pipeline-result-332
Aug 2, 2026
Merged

Return a typed PipelineResult with an execution trace (#332)#442
jeremymanning merged 3 commits into
mainfrom
feat/pipeline-result-332

Conversation

@jeremymanning

Copy link
Copy Markdown
Member

Implements #332's result contract. The result.qc() half of that issue is not here — see the end.

Correcting something I told you

I said declared outputs were "parsed, never resolved". That was wrong_extract_outputs renders them, and has all along.

The actual defect is worse and less visible: declared outputs changed the shape of the return value.

# pipeline without outputs:
{"write_note": {...}, "read_back": {...}}

# same method, pipeline with outputs:
{"steps": {...}, "outputs": {...}}

Two shapes from one method, distinguishable only by inspecting keys — and a pipeline with a step called outputs collided with the second outright.

Mostly recovering discarded state, not new machinery

Field Already existed on Was
per-step status / error Task.status / .error discarded
step timing Task.started_at / .completed_at discarded
retries Task.retry_count discarded
dependency + execution order Pipeline.get_execution_levels() discarded
run identity + start executor context discarded

All of it was computed, then dropped at return final_result.

PipelineResult is a Mapping, so result["step_id"] returns the raw value exactly as before — the several thousand assertions that index results directly are untouched. The trace arrives as attributes.

status and success are different questions — and conflating them was a live bug

status records whether the task finished; success whether it worked. A tool returning {"success": False} without raising does finish.

My first implementation selected failures on status == FAILED, and the golden exit-code test went green when it should have been red — a pipeline with a failing step exiting 0. That is precisely the failure mode this project keeps having to re-fix, and it reappeared inside the change meant to make failures legible.

StepResult.success now accounts for both, and the CLI's exit code consults it.

Serialisation

The CLI emitted json.dumps(results, default=str). That default= silently stringified anything unserialisable, so the output was neither stable nor round-trippable — CLI/API equivalence was impossible to assert, not merely unasserted.

It now serialises to_dict(), which needs no coercion. normalized() drops the execution id and wall-clock times — the only fields that legitimately differ between runs — and the golden test compares whole normalised documents from both surfaces, as you asked, rather than one nested value.

Test expectations that changed, and why none is a weakening

  1. Golden/CLI tests read the typed document instead of the old flat map. The contract changed by request.
  2. My own assertion that a failed step has status == "failed" contradicted the status/success distinction this PR introduces. The code was right; my expectation was wrong.
  3. A len(stdout.splitlines()) < 30 ceiling stood in for "no log noise" while the payload was compact. The typed document is legitimately longer, so that bound had started measuring the document rather than the noise. Round-tripping the whole of stdout already proves there is no interleaved logging; an explicit check for DEBUG/INFO/WARNING/Traceback replaces the magic number.

Numbers

before after
Blocking suite 476 passed 490 passed, 13 skipped, 0 failed, 0 xfailed
Collection 3271 3285
ruff / uv lock --check / git diff --check / action-docs drift clean

Not in this PR

result.qc()#332 also asks for an orchestrator model to grade output quality. That is a quality-evaluation feature, not a result contract, and it cannot sit inside a deterministic acceptance contract; your own quality section says an LLM judge "may supplement — but never replace" deterministic scoring. It wants its own issue.

Fallback events. retries is a count, and skips are recorded, but nothing in the runtime currently records why a fallback happened — there is no emission point to read. Adding one is a change to the execution path rather than to the result type, so it belongs with whatever introduces fallback policy.

provider is read from the step envelope and is currently None for every model step, because the envelope records model_used and no provider. The field is in the contract and the plumbing is one line at the point the envelope is built — but that line belongs with the model-selection work, not smuggled in here.

execute_pipeline returned a bare {step_id: value} dict -- except when the
pipeline declared `outputs:`, when it returned {"steps":..., "outputs":...}
instead. Two shapes from one method, distinguishable only by inspecting
keys, and a pipeline with a step called `outputs` collided with the
second outright.

Almost everything else the run knew was computed and then discarded at
the return statement. Task has carried status, started_at, completed_at,
retry_count, dependencies and the error all along; Pipeline can already
report dependency levels; execution_id and start_time were sitting in
the executor's context. PipelineResult keeps them.

It is a Mapping, so result["step_id"] returns the step's raw value
exactly as before and the several thousand existing assertions that
index results directly are unaffected. The trace arrives as attributes:
status, success, outputs, steps, execution_order, execution_levels,
timing, and failed/skipped/retried views. Each StepResult carries its
action, status, success, value, structured error and error_type, the
tool or model and provider that ran it, start/end/duration, retries and
dependencies.

status and success answer different questions, and conflating them was a
real defect. status records whether the task finished; success whether
it worked. A tool returning {"success": False} without raising *does*
finish, so selecting failures on status alone reported a failing
pipeline as successful -- caught here by the golden exit-code test going
green when it should have been red. StepResult.success accounts for
both, and the CLI's exit code consults it.

The CLI now serialises to_dict() rather than json.dumps(result,
default=str). That `default=str` silently stringified anything
unserialisable, so the JSON was neither stable nor round-trippable and
CLI/API equivalence could not be asserted at all. normalized() drops the
execution id and wall-clock times -- the only fields that legitimately
differ between runs -- and the golden test now compares whole normalised
documents from both surfaces instead of one nested value.

Three test expectations changed with the contract, none weakened:

  - the golden and CLI tests read the typed document rather than the old
    flat map;
  - my own assertion that a failed step has status "failed" contradicted
    the status/success distinction this PR introduces, and was wrong;
  - a `len(stdout.splitlines()) < 30` ceiling stood in for "no log noise"
    while the payload was compact. The typed document is legitimately
    longer, so that bound measured the document rather than the noise.
    Round-tripping the whole of stdout already proves the point, and an
    explicit check for log markers replaces the magic number.

Blocking suite 476 -> 490 passed, 13 skipped, 0 failed, 0 xfailed.
Collection 3271 -> 3285. ruff, uv lock --check, git diff --check and the
generated action docs all clean.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@jeremymanning

Copy link
Copy Markdown
Member Author

CI 9/9 green, and the legacy backlog improved.

Via the annotation route (repos/OWNER/REPO/check-runs/<job-id>/annotations), comparing against the post-merge main run rather than a PR-branch one:

main @ post-#441 this PR
failed 475 473
passed 1845 1847
errors 189 189
skipped 200 200
total red 664 662

Two legacy tests recovered, errors unchanged, nothing regressed. deselected moves 575 → 589 (+14), which is this PR's new contract/e2e-marked tests being excluded from the legacy selection by construction.

The two recoveries are plausibly tests that asserted on a failing step and previously got the error handler's retry policy where the reason belonged — but I have not attributed them individually, and I am not going to claim it without the per-test diff.

Stacked on #442, which the routing tests use to assert skipped steps
through the typed result.

`on_false` and `on_success` did not exist at all. `condition:` was
already evaluated and already skipped a step, so what was missing was
purely the routing: where to go once the answer is known.

Routing jumps forward and marks the steps in between as skipped, reusing
`_skip_tasks_between` -- the machinery `goto` already had -- rather than
adding a second one beside it. Skipping is not failing: a pipeline that
routed around a step still reports success, and PipelineResult separates
skipped_steps from failed_steps accordingly.

The awkward part is `on_failure`, which already meant a failure *policy*
(fail / continue / skip / retry) and now also names a step to jump to.
Both readings are supported and disambiguated by value: a reserved policy
word keeps its policy meaning, anything else is a step id, and when it
names a step the run continues there instead of aborting.

That is only safe because the genuinely ambiguous case is refused rather
than guessed. A pipeline with a step whose id *is* a policy word -- a
step called `retry` -- fails to compile, because `on_failure: retry`
could not be told apart from selecting the policy. core/routing.py holds
that single definition; the compiler and the executor both consume it.

Routing targets are validated at compile time: a jump to a step that does
not exist, or a step routing to itself, is exit 2 naming the target.
That check went into validation/dependency_validator.py, which is the
validator actually on the compile path -- my first attempt put it in
compiler/schema_validator.py, whose validate_dependencies() nothing
calls, so the tests failed while the code looked right. Only the schema
widening stayed there: `on_failure` was constrained to an enum of the
four policies, which would have rejected every routing target outright.

The runtime decision uses StepResult.success, not TaskStatus. A tool
returning {"success": False} without raising leaves its task COMPLETED,
so routing on status alone sent a failing step down the on_success path
-- the same distinction #442 had to draw, reused here rather than
restated a third time.

The contract is updated: ADR 0001 gains a control-flow section, and
docs/actions.md is generated with the routing table and the policy list
so the published vocabulary cannot drift from FAILURE_POLICIES.

Blocking suite 490 -> 506 passed, 13 skipped, 0 failed, 0 xfailed.
Collection 3285 -> 3301. Examples validating unchanged at 3 of 111.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Implement control-flow routing: on_false, on_success, on_failure (#333)
@jeremymanning
jeremymanning merged commit be2e262 into main Aug 2, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant